Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mailer] Changed EventDispatcherInterface dependency from Component to Contracts #31956

Merged
merged 1 commit into from Jun 11, 2019

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Jun 8, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? almost yes, see #31956 (comment)
Fixed tickets -
License MIT
Doc PR -

Follow up of #31946 (comment) . I hope this kind of changes are allowed for experimental components.

BTW, @nicolas-grekas , why Psr14 interface is optional for Contract's `EventDispatcherInterface https://github.com/symfony/symfony/blob/4.4/src/Symfony/Contracts/EventDispatcher/EventDispatcherInterface.php#L16 ?

@Koc Koc force-pushed the mailer-change-event-dispatcher-dependency branch from e3efc9e to 848fce2 Compare June 8, 2019 12:53
@nicolas-grekas nicolas-grekas added this to the next milestone Jun 8, 2019
@nicolas-grekas nicolas-grekas changed the title Changed EventDispatcherInterface dependency from Component to Contracts [Mailer] Changed EventDispatcherInterface dependency from Component to Contracts Jun 8, 2019
@nicolas-grekas
Copy link
Member

Makes sense to me, don't miss bumping the bridge's lowest version in composer.json

why Psr14 interface is optional for Contract's `EventDispatcherInterface

technically: because we are >=PHP7.1 while PSR14 is >=7.2
also because PSR14 doesn't need to be a hard requirement.

@Koc Koc force-pushed the mailer-change-event-dispatcher-dependency branch from d39cada to bdb6217 Compare June 9, 2019 08:03
@Koc
Copy link
Contributor Author

Koc commented Jun 9, 2019

@nicolas-grekas I've updated minimum required versions for both Bridge -> Mailer and Mailer -> Bridge, but deps=high job still fails because current dev-master hasn't my changes. How can I fix that or should we just ignore this failed job?

@nicolas-grekas
Copy link
Member

How can I fix that or should we just ignore this failed job?

deps=high can fail in this situation, until the patch is merged to master.

@fabpot
Copy link
Member

fabpot commented Jun 11, 2019

Thank you @Koc.

@fabpot fabpot merged commit bdb6217 into symfony:4.4 Jun 11, 2019
fabpot added a commit that referenced this pull request Jun 11, 2019
…rom Component to Contracts (Koc)

This PR was merged into the 4.4 branch.

Discussion
----------

[Mailer] Changed EventDispatcherInterface dependency from Component to Contracts

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | almost yes, see #31956 (comment)
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Follow up of #31946 (comment) . I hope this kind of changes are allowed for experimental components.

BTW, @nicolas-grekas , why Psr14 interface is optional for Contract's `EventDispatcherInterface https://github.com/symfony/symfony/blob/4.4/src/Symfony/Contracts/EventDispatcher/EventDispatcherInterface.php#L16 ?

Commits
-------

bdb6217 Changed EventDispatcherInterface dependency from Component to Contracts
4.4.0
-----

* [BC BREAK] Transports depend on `Symfony\Contracts\EventDispatcher\EventDispatcherInterface`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not actually a BC break if the component interface extends the contracts one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interface from component has methods like addListener, addSubscriber but interface from contracts has only dispatch method. So if some of transports call addListener - now it could get fatal error.

@Koc Koc deleted the mailer-change-event-dispatcher-dependency branch June 11, 2019 20:13
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants